Introduce the "Prefer absolute timestamps" setting for users that want to disable relative time #24342

yardenshoham · 2023-04-25T21:02:57Z

Resolves viewing dates requires mouse hover #22493
Resolves add preference setting for commit time display format #4520

Apparently, some users prefer all timestamps to display the full date instead of relative time. They can do that now by setting it in their appearance preferences.

This setting is set to false by default, allowing dates to render as "5 hours ago". Here are some screenshots of the UI with this setting set to true:

Changes

Refactor a bit to get context into TimeSince, pass context in templates that call TimeSince
Allow extra attributes in DateTime so we can pass the time-since class
Add a new setting in the user's appearance page

silverwind · 2023-04-25T22:31:14Z

How about TIME_FORMAT = relative | absolute? That would leave room for other formats and is more clear of an option imho. Should note that it does not affect all dates, as some are still absolute regardless of setting.

yardenshoham · 2023-04-25T22:33:02Z

I can't think of other possible values except the ones you mentioned

silverwind · 2023-04-25T22:38:50Z

Hmm, I would invert the setting at least and name it like "Force Absolute Timestamps". Settings which default to false seem better in general.

Also, I think ideally this would be a per-user setting, not per-server because as you said, it's a user preference, not an admin preference.

silverwind · 2023-04-25T23:21:46Z

I think it may be worth it to introduce a block in the user appearance settings for options like this, stored server-side. I have a few other settings in mind for this as well, like tab size preference.

wxiaoguang · 2023-04-26T01:07:22Z

I think we can postpone this setting to per-user setting. And we might have found a perfect context-function solution at that time.

yardenshoham · 2023-04-26T08:45:42Z

What's the process for adding a setting to the user's appearance page?

wxiaoguang · 2023-04-26T08:47:30Z

What's the process for adding a setting to the user's appearance page?

"progress": There are already some setting, eg: hide some comments

yardenshoham · 2023-04-26T08:48:58Z

Do I need to add a migration? Any PR I can look at to model mine?

wxiaoguang · 2023-04-26T08:50:12Z

Do I need to add a migration? Any PR I can look at to model mine?

No migration is needed. The user setting system is flexible

yardenshoham · 2023-04-26T08:50:49Z

I will try

silverwind · 2023-04-26T09:03:31Z

Thanks, I'm looking forward to it. It'll be helpful to have as a reference for future PRs that add user settings.

confusedsushi · 2023-04-26T14:07:20Z

I would love to see this soon in Gitea. A per user setting is appreciated, but being able to set this server wide is much more important to me.

…e setting Signed-off-by: Yarden Shoham <[email protected]>

yardenshoham · 2023-04-26T21:16:25Z

I managed to add the setting to the appearance page but I now need the context user in TimeSince so I can load the setting. How can I get it? I need it in since.go

wxiaoguang · 2023-04-27T03:06:14Z

I think we can postpone this setting to per-user setting. And we might have found a perfect context-function solution at that time.

Need "a perfect context-function solution" #24228

silverwind · 2023-04-27T06:45:41Z

I would love to see this soon in Gitea. A per user setting is appreciated, but being able to set this server wide is much more important to me.

Not sure we want it. Personal preference is important and I wouldn't want to see a admin forcing one particular style onto users. What can be global is a default style, but users need to be able to change it.

yardenshoham · 2023-04-28T09:39:21Z

I don't think the problem is context now. I have an import cycle, how can I avoid it?

confusedsushi · 2023-04-28T09:46:16Z

I would love to see this soon in Gitea. A per user setting is appreciated, but being able to set this server wide is much more important to me.

Not sure we want it. Personal preference is important and I wouldn't want to see a admin forcing one particular style onto users. What can be global is a default style, but users need to be able to change it.

In company environments forcing user settings is often the better choice. For a company it is important that everyone is using the tools the same way. Documentation how tools have to be configured is often not read carefully. If an admin can force certain settings you can avoid misconfiguration.

Nevertheless, I appreciate that setting, even if every user needs to configure it by its own.

wxiaoguang · 2023-04-28T09:52:40Z

I don't think the problem is context now. I have an import cycle, how can I avoid it?

I would still recommend to wait for a perfect context support.

The last time for such "context passing trick" is the avatar, it causes a lot of 500, not only one PRs were trying to fix these bugs.

yardenshoham · 2023-04-28T09:59:02Z

Even if I wait for that, I would still have the issue with the import cycle, right?

options/locale/locale_en-US.ini

Co-authored-by: silverwind <[email protected]>

silverwind · 2023-04-28T21:31:32Z

@yardenshoham what do you think about switchting to string option absolute | relative (e.g. dropdown) and adding the ui.DEFAULT_TIMESTAMP_FORMAT to set the default? I think it's just more understandable than a boolean option.

yardenshoham · 2023-04-28T21:33:37Z

The thing is, if you choose relative, some dates will still display as absolute so it might confuse users

silverwind · 2023-04-28T21:36:29Z

Right, it's not used everywhere. Let's rename the vars and such to "absoluteTimestamps" and if you agree, also add the setting ui.DEFAULT_ABSOLUTE_TIMESTAMPS to obtain the default and satisfy the request above.

yardenshoham · 2023-04-28T21:37:19Z

Sounds good, I'm on it

wxiaoguang · 2023-04-29T02:18:16Z

There are some possible choices:

Do we need a global ini setting?
Do we want to support per-user setting?
Do we want to avoid using $.Context in templates?

For "global setting", I think it's better to have a new inheritable config system (but not ini): global setting -> org/user setting -> repo setting, instead of adding many global ini options (maintainability?), there will be a lot of similar requirements: global-2FA/org-2FA, global merge style / org merge style, etc.

If we agree for choice-2, then choice-1 seems not necessary, and we had better do choice-3.

My personal opinion is: at the moment only do 2+3 (per-user + wait for template context function)

What do you think?

silverwind · 2023-04-29T06:56:22Z

Yes I agree global setting is not needed in case we are planning for a better config system later which also covers per-org settings. I'm still of the opinion that per-user setting is enough as a start here, and admins don't really need "power" over this imho, as it seems like power abuse to force stylistic choices onto users.

So let's just rename and then I'm happy to approve.

yardenshoham · 2023-04-29T10:21:20Z

templates/repo/release/list.tmpl

@@ -76,7 +76,7 @@
 									{{$.locale.Tr "repo.released_this"}}
 								</span>
 								{{if .CreatedUnix}}
-									<span class="time">{{TimeSinceUnix .CreatedUnix $.locale}}</span>
+									<span class="time">{{TimeSinceUnix $.Context .CreatedUnix $.locale}}</span>


Why does this not work when a user is unauthenticated but nil is fine?

Suggested change

{{TimeSinceUnix $.Context .CreatedUnix $.locale}}

{{TimeSinceUnix nil .CreatedUnix $.locale}}

Shouldn't, in the worse case, $.Context be nil?

The error:

Render failed, failed to render template: repo/release/list, error: template error: builtin(static):repo/release/list:80:30 : executing "repo/release/list" at <TimeSinceUnix $.Context .CreatedUnix $.locale>: error calling TimeSinceUnix: runtime error: invalid memory address or nil pointer dereference ---------------------------------------------------------------------- {{TimeSinceUnix $.Context .CreatedUnix $.locale}} ^ ----------------------------------------------------------------------

Haven't looked into details, just share some my experiences:

Golang template is not friendly to panic text/template: provide full stacktrace when the function call panics golang/go#59400 , you need to write some temp code to do "recover" to see what happens.

According to Golang standard, ctx should never be nil. The Golang standard library functions will panic if you feed nil to them.

wxiaoguang · 2023-04-29T14:07:32Z

If we are not in a hurry, I would suggest to leave enough time for this PR.

I wouldn't put Request Changes, but there are indeed some problems which need enough attention:

The "context" problem, it's not safe to fill .Context in templates. It's better to avoid doing so. Comment Introduce the "Prefer absolute timestamps" setting for users that want to disable relative time #24342 (comment)
The performance, think about a case, there are 100 datetime components on a page, then the GetSetting will send 100 database queries, which is very unfriendly to database and slows down the whole system.

yardenshoham · 2023-04-29T14:09:23Z

I understand and agree

silverwind · 2023-04-29T14:10:12Z

The performance, think about a case, there are 100 datetime components on a page, then the GetSetting will send 100 database queries, which is very unfriendly to database and slows down the whole system.

Right, it should be one database query per request, maybe even with an additional caching mechanism on top.

wxiaoguang · 2023-04-29T14:19:44Z

I am doing my best to improve the template system now, hopefully we can start the "per-user setting for all templates" work soon.

Thank you! 🙏

yardenshoham · 2023-04-29T14:20:20Z

If you want we could go back to the server-wide option

wxiaoguang · 2023-04-29T14:22:43Z

If I failed to get the context function working before 1.20 frozen, let's go back to the global ini option solution, I also agree that it brings true values for end users. 👍

silverwind · 2023-04-29T18:24:08Z

Still I don't quite understand why this was closed as the issues would surely have been fixable, or does it really depend on this upcoming context rewrite?

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 25, 2023

pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 25, 2023

yardenshoham added the type/enhancement An improvement of existing functionality label Apr 25, 2023

yardenshoham added this to the 1.20.0 milestone Apr 25, 2023

yardenshoham marked this pull request as draft April 26, 2023 15:53

yardenshoham changed the title ~~Introduce ALLOW_RELATIVE_TIME setting for users that want to disable relative time~~ Introduce the "Force absolute timestamp" setting for users that want to disable relative time Apr 26, 2023

Change to "Force absolute timestamps" and add to the user's appearanc…

569b131

…e setting Signed-off-by: Yarden Shoham <[email protected]>

yardenshoham force-pushed the issues/22493 branch from 44db04c to 569b131 Compare April 26, 2023 21:16

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 26, 2023

How can I avoid the import cycle?

9aaad2b

silverwind reviewed Apr 28, 2023

View reviewed changes

options/locale/locale_en-US.ini Outdated Show resolved Hide resolved

Update options/locale/locale_en-US.ini

0479818

Co-authored-by: silverwind <[email protected]>

yardenshoham added 2 commits April 29, 2023 09:15

Use Prefer instead of Force

fb5c5da

Merge remote-tracking branch 'upstream/main' into issues/22493

913b90b

yardenshoham changed the title ~~Introduce the "Force absolute timestamp" setting for users that want to disable relative time~~ Introduce the "Prefer absolute timestamps" setting for users that want to disable relative time Apr 29, 2023

yardenshoham commented Apr 29, 2023

View reviewed changes

yardenshoham added 2 commits April 29, 2023 10:27

Share the default

c98fcfb

Merge remote-tracking branch 'upstream/main' into issues/22493

3d14785

silverwind approved these changes Apr 29, 2023

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 29, 2023

yardenshoham closed this Apr 29, 2023

yardenshoham deleted the issues/22493 branch April 29, 2023 14:09

wxiaoguang mentioned this pull request Apr 29, 2023

text/template: provide full stacktrace when the function call panics golang/go#59400

Open

GiteaBot removed this from the 1.20.0 milestone Apr 29, 2023

wxiaoguang mentioned this pull request May 1, 2023

Proof of concept: Template context function #24457

Closed

go-gitea locked as resolved and limited conversation to collaborators Jul 31, 2023

	<span class="time">{{TimeSinceUnix $.Context .CreatedUnix $.locale}}</span>
	<span class="time">{{TimeSinceUnix nil .CreatedUnix $.locale}}</span>

Introduce the "Prefer absolute timestamps" setting for users that want to disable relative time #24342

Introduce the "Prefer absolute timestamps" setting for users that want to disable relative time #24342

Conversation

yardenshoham commented Apr 25, 2023 • edited by delvh Loading

Changes

silverwind commented Apr 25, 2023 • edited Loading

yardenshoham commented Apr 25, 2023

silverwind commented Apr 25, 2023 • edited Loading

silverwind commented Apr 25, 2023

wxiaoguang commented Apr 26, 2023

yardenshoham commented Apr 26, 2023

wxiaoguang commented Apr 26, 2023

yardenshoham commented Apr 26, 2023

wxiaoguang commented Apr 26, 2023 • edited Loading

yardenshoham commented Apr 26, 2023

silverwind commented Apr 26, 2023 • edited Loading

confusedsushi commented Apr 26, 2023

yardenshoham commented Apr 26, 2023

wxiaoguang commented Apr 27, 2023 • edited Loading

silverwind commented Apr 27, 2023

yardenshoham commented Apr 28, 2023

confusedsushi commented Apr 28, 2023

wxiaoguang commented Apr 28, 2023

yardenshoham commented Apr 28, 2023

silverwind commented Apr 28, 2023

yardenshoham commented Apr 28, 2023

silverwind commented Apr 28, 2023 • edited Loading

yardenshoham commented Apr 28, 2023

wxiaoguang commented Apr 29, 2023 • edited Loading

silverwind commented Apr 29, 2023 • edited Loading

yardenshoham Apr 29, 2023

Choose a reason for hiding this comment

yardenshoham Apr 29, 2023

Choose a reason for hiding this comment

yardenshoham Apr 29, 2023

Choose a reason for hiding this comment

wxiaoguang Apr 29, 2023

Choose a reason for hiding this comment

wxiaoguang commented Apr 29, 2023 • edited Loading

yardenshoham commented Apr 29, 2023

silverwind commented Apr 29, 2023

wxiaoguang commented Apr 29, 2023

yardenshoham commented Apr 29, 2023

wxiaoguang commented Apr 29, 2023 • edited Loading

silverwind commented Apr 29, 2023

yardenshoham commented Apr 25, 2023 •

edited by delvh

Loading

silverwind commented Apr 25, 2023 •

edited

Loading

silverwind commented Apr 25, 2023 •

edited

Loading

wxiaoguang commented Apr 26, 2023 •

edited

Loading

silverwind commented Apr 26, 2023 •

edited

Loading

wxiaoguang commented Apr 27, 2023 •

edited

Loading

silverwind commented Apr 28, 2023 •

edited

Loading

wxiaoguang commented Apr 29, 2023 •

edited

Loading

silverwind commented Apr 29, 2023 •

edited

Loading

wxiaoguang commented Apr 29, 2023 •

edited

Loading

wxiaoguang commented Apr 29, 2023 •

edited

Loading